Skip to content

Make tracing Optional for bevy_utils #15879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

Make tracing Optional for bevy_utils #15879

wants to merge 3 commits into from

Conversation

@bushrat011899
Copy link
Contributor

@bushrat011899 bushrat011899 commented Oct 13, 2024 •
edited
Loading

Objective

Solution

  • Made tracing optional (behind the tracing feature, enabled by default).
  • Switched to using log within bevy_utils, and exported it as tracing.

Testing

  • cargo build --target thumbv4t-none-eabi

Migration Guide

If you were using bevy_utils with default features disabled, and you relied on the tracing re-export, and you relied on a tracing item that is not available in log, enable the tracing feature.

Notes

tracing has built-in support for capturing logs from log and this is enabled by default. The biggest loss going from log to tracing is the loss of info_span/etc. Otherwise, the crates are largely identical in behaviour, with log having no dependencies and more popularity on crates.io.

@bushrat011899 bushrat011899 added D-Trivial Nice and easy! A great choice to get started with Bevy C-Dependencies A change to the crates that Bevy depends on A-Utils Utility functions and types X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 13, 2024
@bushrat011899 bushrat011899 added this to the 0.16 milestone Oct 13, 2024
@bushrat011899 bushrat011899 force-pushed the BevyUtilsTracingFeature branch from 95c0c68 to bb237d8 Compare October 15, 2024 08:18
@alice-i-cecile
Copy link
Member

I don't quite follow the strategy here. It looks like we're unconditionally calling the log variants, but we want to call the tracing ones if and only if the feature is enabled (just like before).

@bushrat011899 @alice-i-cecile
Only use log if required
c171598 
Co-Authored-By: Alice Cecile <[email protected]>
alice-i-cecile
alice-i-cecile reviewed Oct 16, 2024
View reviewed changes
@bushrat011899
Copy link
Contributor Author

I've also updated the migration guide since this is now only just barely a breaking change.

@alice-i-cecile alice-i-cecile added M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 16, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 16, 2024
@Elabajaba
Copy link
Contributor

Just a small sidenote, log has a much higher performance overhead than tracing for spammy logs and log filtering (see gfx-rs/wgpu#6046 (comment) for why).

@mockersf
Copy link
Member

mockersf commented Oct 19, 2024 •
edited
Loading

I think the re-exporting of log as tracing should be avoided.

I would prefer to take the breaking change in this PR, and re-exports the common functions and macros between tracing and log as bevy_utils::log, and use that through the engine

@bushrat011899
Copy link
Contributor Author

These are both excellent points. I will update the documentation to indicate that users should prefer to use the tracing feature unless they must disable it. As for the breaking change, I agree. I will update the PR to expose the common items in log and tracing from bevy_utils::log. For items which are exclusively available from tracing, should we export those from bevy_utils::log, only export them from bevy_utils::tracing, or re-export the whole tracing module (causing items like trace! to be exported twice)?

I'm leaning towards only exporting tracing exclusive items from bevy_utils::tracing, and everything else from bevy_utils::log.

@bushrat011899 bushrat011899 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+1 more reviewer

@BenjaminBrienen BenjaminBrienen BenjaminBrienen approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Utils Utility functions and types C-Dependencies A change to the crates that Bevy depends on D-Straightforward Simple bug fixes and API improvements, docs, test and examples D-Trivial Nice and easy! A great choice to get started with Bevy M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged X-Uncontroversial This work is generally agreed upon

Projects

None yet

Milestone

0.16

Development

Successfully merging this pull request may close these issues.

5 participants

@bushrat011899 @alice-i-cecile @Elabajaba @mockersf @BenjaminBrienen